Conversation
idank
left a comment
idank
left a comment
There was a problem hiding this comment.
I did a pass and it generally looks good to me. Granted I haven't touched the parser in years so we're learning together. :-)
Left a few small comments, let's fix those and add some tests? Thanks for the contribution!
bashlex/parser.py
Outdated
| | newline_list LEFT_PAREN pattern RIGHT_PAREN newline_list''' | ||
| handleNotImplemented(p, 'pattern list') | ||
|
|
||
| parserobj = p.context |
bashlex/parser.py
Outdated
| rparen = ast.node(kind='reservedword', word=p[4], pos=p.lexspan(4)) | ||
| parts.extend([lparen, patterns, rparen]) | ||
| if p.slice[-1].type == "compound_list": | ||
| # for some reason, p[-1] does not give the "true" last element, do not know why |
There was a problem hiding this comment.
|
I've pushed my updated code which I believe to be correct (although resulting in a somewhat unintuitive AST, in my opinion, but it seems right). Locally, "make tests" doesn't work, it gives an error in the "malformed if" case, which is odd since I didn't touch that, to my knowledge. |
idank
left a comment
idank
left a comment
There was a problem hiding this comment.
looks good overall!
- can you please look at the test failures
- please squash the commits, i think you want to end up with one?
bashlex/parser.py
Outdated
| redirects = [], | ||
| list=[ast.node(kind='case', parts=parts, pos=_partsspan(parts))], | ||
| pos=_partsspan(parts)) | ||
| #handleNotImplemented(p, 'case command') |
| '''case_clause : pattern_list | ||
| | case_clause_sequence pattern_list''' | ||
| handleNotImplemented(p, 'case clause') | ||
|
|
| | newline_list LEFT_PAREN pattern RIGHT_PAREN compound_list | ||
| | newline_list LEFT_PAREN pattern RIGHT_PAREN newline_list''' | ||
| handleNotImplemented(p, 'pattern list') | ||
|
|
| patterns = p[3] | ||
| rparen = ast.node(kind='reservedword', word=p[4], pos=p.lexspan(4)) | ||
| parts.extend([lparen, patterns, rparen]) | ||
| if p.slice[-1].type == "compound_list": |
There was a problem hiding this comment.
please add a comment that explains this part
| '''pattern : WORD | ||
| | pattern BAR WORD''' | ||
| handleNotImplemented(p, 'pattern') | ||
|
|
| ;; | ||
| esac | ||
| """ | ||
| self.assertASTEquals(s, |
There was a problem hiding this comment.
i see what you mean about the resulting ast :) is there a more condensed way to represent it, that is still in line with how everything else is parsed?
|
I'm not sure what is going on with the test failures, I'm still looking into it, but the failures are in parts of the code that I hadn't knowingly touched (malformed if statements). I'll make sure to squash the commits on the next one (I'm pretty sure there are going to be more changes). |
2 participants
Made some changes to parser and ast to support case_clauses and patterns. Submitting as a draft PR because I'm not sure if it is correct (it looks correct to me on the example I tested on, but I don't know how to compare it to the "canonical" ast).